feat(deleter): clear or report org delete blockers up front - #1857
feat(deleter): clear or report org delete blockers up front#1857whoAbhishekSah wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughOrganization deletion now performs billing preflight checks, aggregates structured blockers, and accepts token-forfeit acknowledgement. Billing-account deletion audits forfeited tokens. The Connect API returns structured failed-precondition details for blocked deletions. ChangesOrganization deletion workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
29a0e18 to
783da24
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 51d78c26-5a73-43e9-8050-b9aec5ac46cb
⛔ Files ignored due to path filters (1)
proto/v1beta1/frontier.pb.gois excluded by!**/*.pb.go,!proto/**
📒 Files selected for processing (13)
Makefilebilling/invoice/invoice.gocore/audit/audit.gocore/deleter/deleter.gocore/deleter/mocks/credit_service.gocore/deleter/mocks/subscription_service.gocore/deleter/service.gocore/deleter/service_test.gogo.modinternal/api/v1beta1connect/deleter.gointernal/api/v1beta1connect/deleter_test.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/cascade_deleter.go
783da24 to
3c17128
Compare
3c17128 to
969ef16
Compare
Coverage Report for CI Build 32462377017Coverage increased (+0.1%) to 48.912%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
969ef16 to
7117b6a
Compare
7117b6a to
b479ddd
Compare
b479ddd to
2711e64
Compare
efd1fca to
0d92025
Compare
0d92025 to
f8575b8
Compare
whoAbhishekSah
left a comment
There was a problem hiding this comment.
Reviewed the deleter changes; 10 inline notes below. The two biggest for me: provider-drifted subs making an org permanently undeletable, and draft invoices slipping through the blocker check and getting discarded by the customer delete. None of these are fixed by the later commits in the stack.
f8575b8 to
3b1e2c8
Compare
rohilsurana
left a comment
There was a problem hiding this comment.
Stack-aware review (base org-delete-preflight-mocks). A few things to flag. The invoice one is the most important; the rest are smaller. I dropped two earlier notes after reading the stack: the empty-PlanID-is-paid case is a documented choice, and the mid-teardown balance read is reworked in #1880.
3b1e2c8 to
4664db3
Compare
4664db3 to
1d55f24
Compare
|
Round 3 sandbox results — retesting the behavior changes from the review (stack: #1857 → #1880 → #1894, tested at the tip 1. Credit-note invoice no longer blocks. Draft invoice with total −500 (amount_remaining 0). 2. Credit-balance-covered invoice no longer blocks. Customer with a 1000 credit balance and a draft invoice of total 800 (amount_due 0 on Stripe). Check passed and the delete succeeded. 3. Dangling plan yields a blocker, not an error. Subscription row pointing at a plan id that no longer exists (a real Stripe sub behind it). Check and delete both returned the 4. Cancel pass is two-phase. One org, two billing accounts: A with an active free-plan subscription, B with an open unpaid invoice. The delete blocked on B's invoice and A's subscription stayed active on Stripe and in the local rows. After voiding the invoice, the retry deleted the org and canceled A's subscription itself. Both Stripe customers were removed. 5. Retry recovers the forfeit notice from audit. Org with zero live balance plus an All test orgs, billing accounts, and Stripe customers were cleaned up afterwards. 🤖 Generated with Claude Code |
rohilsurana
left a comment
There was a problem hiding this comment.
Re-review after the fix commits. The earlier points are handled well: AmountRemaining for the filter, the unresolvablePlanBlocker for dangling plans, the judge-before-cancel restructure, and the test comment. A few new things the fixes surfaced, plus two small doc nits.
1d55f24 to
896fb62
Compare
896fb62 to
b34ac05
Compare
The delete first checks everything that blocks it and returns all the reasons together as one failed_precondition response: a running subscription on a paid plan (the caller downgrades it to the standard plan), invoices that still ask for money, and a negative token balance which support has to settle. Plans resolve lazily, only when a running subscription references one. When nothing blocks, subscriptions still running on a free plan are canceled immediately with unbilled usage invoiced on the spot — tolerating copies already gone on the provider — and the invoice check runs again so a final invoice still blocks. The plan is judged again in that pass, so a paid subscription created mid-delete blocks instead of being canceled. Unused tokens do not block: the delete forfeits them and writes the amount to an audit record. An already-deleted org returns not found before any checks run. Invoices are judged straight from the billing provider through a new cheap ListPayableOnProvider (three status-filtered pages, no local writes) instead of a full sync, and drafts with a non-zero amount now block too: the provider finalizes them shortly, and deleting inside that window would silently lose the charge.
b34ac05 to
5172886
Compare
Closes #1837. Based on #1865; the forfeit email (#1880) and the check RPC (#1893/#1894) stack on top.
DeleteOrganizationnow checks everything that blocks the delete before touching any data, and returns every reason together as onefailed_preconditionresponse with aPreconditionFailuredetail (one violation per blocker). The proto ack field from the issue text was dropped (raystack/proton#497 closed) — unused tokens never block.What blocks the delete:
ACTIVE_SUBSCRIPTION): any running subscription whose plan has a priced product. The caller downgrades it to the standard plan through the normal plan-change flow, then retries. Plans resolve lazily — only when a running subscription references one — so a broken plan reference can only affect orgs that actually hold such a subscription.UNPAID_INVOICE): open and uncollectible invoices the caller can pay, and drafts — the provider finalizes those shortly, and deleting inside that window would silently lose the charge, so they block until payable. "Asks for money" is judged by the provider'samount_remaining, so a negative-total credit note or an invoice fully covered by the customer's credit balance never blocks.NEGATIVE_TOKEN_BALANCE): the account owes tokens (overdraft). Support settles it.What does not block:
app.billing.tokens.forfeitedaudit record. feat(deleter): email the org owners when a delete forfeits tokens #1880 adds the owner email.Other changes:
invoice.ListPayableOnProvider— three status-filtered pages, no expansion, no local writes — instead of a fullSyncWithProvider, keeping the delete's latency independent of the org's invoice history. An invoice the sync has not stored yet is reported by its provider reference.not_foundbefore any checks run. Disabled orgs stay deletable.UncollectibleState; first use oferrdetailsin the repo (google.golang.org/genproto/googleapis/rpcbecomes a direct dependency).🤖 Generated with Claude Code